[ENH] Improved Sparsity Handling #2341

nikicc · 2017-05-25T10:53:45Z

Issue

Fixes #2322.

Blocked on #2734. Blocked on #2736.

Description of changes

DomainConversion now has three extra attributes telling whether X, Y, or metas should be sparse according to the features inside of them. Metas can only be sparse if no StringVariables are present.
Table.from_table determines the density of X, Y, and metas according to domain conversion.
Add to_sparse and to_dense methods to Table.
Reset variables caches to alleviate interdependencies between test files.
Fix showing sparse metas in Table widget.

Includes

Code changes
Tests
Documentation

jerneju · 2017-05-31T10:08:28Z

That PR should fix #2359 .

jerneju

When someone wants to transform two columns from sparse X to dense Y she or he gets only one Y column which has got double the number of rows.

jerneju · 2017-05-31T10:19:25Z

table = Table("iris")
table.X = sp.csr_matrix(table.X)
attributes = table.domain.attributes[:2] + table.domain.class_vars
class_vars = table.domain.attributes[2:4]
newtable = table.transform(Domain(attributes=attributes, class_vars=class_vars))
self.assertEqual(newtable.Y.shape[0], table.Y.shape[0])

nikicc · 2017-06-01T14:44:02Z

@jerneju nice catch. Should be fixed not, please check.

jerneju · 2017-06-01T14:56:06Z

Orange/tests/test_domain.py

@@ -463,6 +463,42 @@ def test_domain_conversion_is_fast_enough(self):

        self.assertLessEqual(time() - start, 1)

+    def test_domain_conversion_sparsity(self):


I usually add something like this:

""" Description. GH-2341 """

lanzagar · 2017-06-02T13:01:24Z

Orange/data/variable.py

+    .. attribute:: sparse
+
+        A flag about sparsity of the variable. When set, the variable suggests
+        it is should be stored in a sparse matrix. 


codecov-io · 2017-06-02T22:13:44Z

Codecov Report

Merging #2341 into master will increase coverage by 0.01%.
The diff coverage is 95.45%.

@@            Coverage Diff            @@
##           master   #2341      +/-   ##
=========================================
+ Coverage   76.09%   76.1%   +0.01%     
=========================================
  Files         338     338              
  Lines       59723   59760      +37     
=========================================
+ Hits        45444   45479      +35     
- Misses      14279   14281       +2

astaric · 2017-11-10T09:54:44Z

Orange/data/domain.py

@@ -63,6 +75,19 @@ def __init__(self, source, destination):
            source.index(var) if var in source
            else var.compute_value for var in destination.metas]

+        def is_sparse(feats):


should_be_sparse?

That shoulds better 👍 Fixed.

astaric · 2017-11-10T10:03:15Z

Orange/data/table.py

@@ -280,45 +280,77 @@ def from_table(cls, domain, source, row_indices=...):

        global _conversion_cache

-        def get_columns(row_indices, src_cols, n_rows, dtype=np.float64,
-                        is_sparse=False):
+        def array_transform(x, to_sparse):


Could these functions be moved someplace else? They seem like general functions that ensure array/column is sparse/dense. Maybe change to a more expressive name.

lanzagar · 2017-11-10T11:08:37Z

Orange/data/domain.py

+        def is_sparse(feats):
+            """ For a matrix to be stored in sparse, more than 2/3 of columns
+            should be marked as sparse and there should be no string columns
+            since Scipy's sparse matrices don't support dtype=object."""


At least the closing triple quotes of a multi-line docstring should be in their own line (PEP257).
(for opening quotes try to mimic other docstrings nearby)

Changed to match the style as in nerby docstrings.

lanzagar · 2017-11-10T11:08:57Z

Orange/data/domain.py

+            """ For a matrix to be stored in sparse, more than 2/3 of columns
+            should be marked as sparse and there should be no string columns
+            since Scipy's sparse matrices don't support dtype=object."""
+            fraction_sparse = sum(f.sparse for f in feats)/max(len(feats), 1)


space around operators

nikicc · 2017-11-10T15:13:30Z

@astaric, @markotoplak I split the functionality of array_transform and column_transform into 4 helper functions (see the last two fixup commints 4410596, 3d1a96d). Do you think that's better?

I haven't moved the functions yet, though. I will do this once we settle on the best approach. Perhaps to some util library?

DomainConversion now has three more attributes `sparse_X`, `sparse_Y` and `sparse_metas`, which suggest whether the resulting matrix should be sparse or dense.

Table.from_table now sets the sparsity of X, Y and metas as determined by DomainConversion.

When we return subarryas, the flag `is_sparse` wasn't considered, but we simpy returned the subarray in it's original format. Also, make sure subarrays aren't flattened to 1d, as it is required for columns.

Calling len on scipy sparse matrices causes the error: `TypeError: sparse matrix length is ambiguous; use getnnz() or shape[0]`.

... to alleviate interdependencies between tests in differente files. Before that, one tests could mark some attributes of iris as sparse which could cause tests in different files to crash due to variable "reusing".

nikicc added the DH2017 label May 25, 2017

nikicc force-pushed the sparsity-from-domain-conversion branch 3 times, most recently from 4b65c4e to 80475e7 Compare May 25, 2017 11:29

jerneju mentioned this pull request May 31, 2017

Table: when Y has only one column it is always dense #2361

Closed

3 tasks

jerneju suggested changes May 31, 2017

View reviewed changes

nikicc force-pushed the sparsity-from-domain-conversion branch from 125a142 to 71fc394 Compare June 1, 2017 14:37

jerneju approved these changes Jun 1, 2017

View reviewed changes

nikicc added this to the 3.4.3 milestone Jun 2, 2017

nikicc force-pushed the sparsity-from-domain-conversion branch 7 times, most recently from 942f527 to 120f92e Compare June 2, 2017 12:37

lanzagar reviewed Jun 2, 2017

View reviewed changes

nikicc force-pushed the sparsity-from-domain-conversion branch 6 times, most recently from 4d20017 to c23bf8d Compare June 2, 2017 22:13

nikicc force-pushed the sparsity-from-domain-conversion branch 2 times, most recently from 8488448 to 68594d1 Compare June 2, 2017 22:58

nikicc removed this from the 3.4.3 milestone Jun 3, 2017

nikicc force-pushed the sparsity-from-domain-conversion branch 3 times, most recently from 67c4efb to 1d9c77a Compare November 10, 2017 09:59

astaric reviewed Nov 10, 2017

View reviewed changes

lanzagar reviewed Nov 10, 2017

View reviewed changes

nikicc force-pushed the sparsity-from-domain-conversion branch from 1d9c77a to 20ae6d6 Compare November 10, 2017 14:26

nikicc force-pushed the sparsity-from-domain-conversion branch 4 times, most recently from e4affcb to 4aa7120 Compare November 17, 2017 09:36

nikicc added 13 commits November 17, 2017 10:39

DomainConversion: Add sparsity suggestions

951fd8e

DomainConversion now has three more attributes `sparse_X`, `sparse_Y` and `sparse_metas`, which suggest whether the resulting matrix should be sparse or dense.

Table: Set density according to DomainConversion

7fcdbde

Table.from_table now sets the sparsity of X, Y and metas as determined by DomainConversion.

Data.utils: Add methods for assuring sparse/dense

bef9c4b

Table.from_table: Obey is_sparse when returning subarrays

7fa9f05

When we return subarryas, the flag `is_sparse` wasn't considered, but we simpy returned the subarray in it's original format. Also, make sure subarrays aren't flattened to 1d, as it is required for columns.

Table: Consider sparsity flags when domain match

6f004ea

Table: Add to_sparse and to_dense methods

332af4e

Table._check_arrays: Don't use len on sparse matrices

204c2d4

Calling len on scipy sparse matrices causes the error: `TypeError: sparse matrix length is ambiguous; use getnnz() or shape[0]`.

TableModel: Fix showing sparse metas

228afd4

Tests: Fixups according to improved sparsity

e57f340

TestTable: Use transform instead of trom_table

a9bd8ca

WidgetTest, WidgetOutputsTestMixin: Reset variables' cache

a0935fa

... to alleviate interdependencies between tests in differente files. Before that, one tests could mark some attributes of iris as sparse which could cause tests in different files to crash due to variable "reusing".

Gui: Fixups according to improved sparsity

8ed204d

Add tests Table.from_table sparsity hadling

7af1181

nikicc force-pushed the sparsity-from-domain-conversion branch from 4aa7120 to 2b8d9a8 Compare November 17, 2017 09:40

PyLint

4bf6a20

nikicc force-pushed the sparsity-from-domain-conversion branch from 2b8d9a8 to 4bf6a20 Compare November 17, 2017 11:16

astaric merged commit 301d13a into biolab:master Nov 20, 2017

nikicc deleted the sparsity-from-domain-conversion branch November 20, 2017 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Improved Sparsity Handling #2341

[ENH] Improved Sparsity Handling #2341

nikicc commented May 25, 2017 •

edited

Loading

jerneju commented May 31, 2017

jerneju left a comment

jerneju commented May 31, 2017 •

edited

Loading

nikicc commented Jun 1, 2017

jerneju Jun 1, 2017

lanzagar Jun 2, 2017

nikicc Jun 2, 2017

codecov-io commented Jun 2, 2017 •

edited

Loading

astaric Nov 10, 2017

nikicc Nov 10, 2017

astaric Nov 10, 2017

lanzagar Nov 10, 2017

nikicc Nov 10, 2017

lanzagar Nov 10, 2017

nikicc Nov 10, 2017

nikicc commented Nov 10, 2017 •

edited

Loading

		@@ -463,6 +463,42 @@ def test_domain_conversion_is_fast_enough(self):

		self.assertLessEqual(time() - start, 1)

		def test_domain_conversion_sparsity(self):

[ENH] Improved Sparsity Handling #2341

[ENH] Improved Sparsity Handling #2341

Conversation

nikicc commented May 25, 2017 • edited Loading

Issue

Description of changes

Includes

jerneju commented May 31, 2017

jerneju left a comment

Choose a reason for hiding this comment

jerneju commented May 31, 2017 • edited Loading

nikicc commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 2, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikicc commented Nov 10, 2017 • edited Loading

nikicc commented May 25, 2017 •

edited

Loading

jerneju commented May 31, 2017 •

edited

Loading

codecov-io commented Jun 2, 2017 •

edited

Loading

nikicc commented Nov 10, 2017 •

edited

Loading